Preserve compound field data backed by unexported field defs in server-side writes#5539
Conversation
The definition-driven file serializer walks a card's stored Definition
to normalize each field on every server-side instance write (POST,
PATCH, /_atomic). The definition store only carries exported classes,
so a compound field typed with an unexported FieldDef has a
fieldOf/ancestorOf code ref whose child definition can't be looked up.
The serializer treated that as "no fields": containsMany entries
serialized to {}, contains values were dropped, and relationships
pathing through such a field were dropped — any edit or sync of such a
card silently wiped its data on disk. Unresolvable-child values now
pass through verbatim.
The PATCH handler also merged patches over the index's pristine_doc.
The index is downstream of the file and can lag it, so a merge over
stale index state silently reverts fields the index hasn't caught up
on. The merge base is now the stored source file, read inside the same
per-realm write lock as the write.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-compound-field-json-object-data-gets-wiped-out
With the stored file as the merge base, a PATCH writes only the fields the file and the patch specify — the index's schema-defaulted nulls are no longer materialized into the file. File-shape expectations drop the materialized cardInfo, and the no-op tests prime once so their assertions measure the canonical-form steady state. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1b89704b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR updates server-side write flows to prevent data loss when serializing compound fields whose child Definition cannot be resolved (notably when the declared type is an unexported FieldDef), and makes PATCH merging use the stored source file as the merge base rather than potentially-stale index state.
Changes:
file-serializerpreserves compound attribute values verbatim when the child definition can’t be resolved, and preserves relationships whose dotted path crosses such a field.- PATCH merge base is switched from index state to the on-disk JSON source (read under the realm write lock), with clearer errors for non-card stored files.
- Adds realm-server tests covering PATCH and
/_atomic?waitForIndex=truebehavior for unresolvable compound field definitions and index lag scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/runtime-common/realm.ts | PATCH reads/parses the stored instance file under the write lock and merges against it instead of index-derived state. |
| packages/runtime-common/file-serializer.ts | Pass-through behavior for unresolvable compound subtrees; relationship preservation for dotted paths crossing such fields. |
| packages/realm-server/tests/card-endpoints-test.ts | Adds PATCH tests asserting compound-field preservation and merge-base correctness when index lags. |
| packages/realm-server/tests/atomic-endpoints-test.ts | Adds /_atomic?waitForIndex=true test asserting compound-field preservation when writing raw source instances. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
normalizeRelationship only derives links from a single-resource data object, so a JSON:API to-many `data: [...]` relationship under an unresolvable compound field was preserved as an empty object and its targets lost. Expand it into the indexed .N keys to-many relationships are stored under, one entry per resource identifier. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
habdelra
left a comment
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] This review focused on the two places a wrong branch silently destroys user data: the serializer's new preserve-vs-drop boundary, and the PATCH merge-base move — both sit directly on the write path.
Bottom line: no blocking issues. Every mechanism claim in the description checks out against the code, and the affected suites pass locally against this branch: card-endpoints-test.ts + atomic-endpoints-test.ts 79/79 green, plus a fresh 60/60 card-endpoints-test.ts run at head 78f80435 covering the to-many expansion and its new assertion.
What lands right:
- The tri-state lives at the correct altitude. Distinguishing "path names nothing the card declares" (drop) from "path crosses a type the definition store cannot hold" (preserve) in
resolveDottedFieldDefis the minimal precise fix; the tempting wrong version — blanket preservation of unknown relationship keys — would have turned typo'd keys into permanent file residue. I verified the boundary is exact (inline confirmation): unknown keys still drop, and resolved-but-missing types still fail the write loudly becauselookupDefinitionthrows rather than returning undefined. - The merge-base move keeps the critical-section shape and simplifies its guarantee: instead of "wait for indexing inside the lock so the next waiter's index read is fresh", the next waiter now reads the same bytes the lock ordered. Write and index flush both complete inside the lock, so the concurrent-PATCH lost-update guarantee holds (inline confirmation has the full trace).
- Sparse files converge PATCH with POST and
/_atomicon stored shape, and GET responses are untouched since they serialize from the index. The tests' priming pattern honestly documents the one-time canonicalization rewrite a hand-authored file gets on first PATCH.
Recommendations (detail in the inline threads):
- Mirror the to-many
data: [...]expansion into the declaredlinksToManyfan-out — the resolvable branch still empties the shape the unresolvable branch now preserves (pre-existing, non-blocking; file-serializer thread). - Pin the corrupt-file
systemErrorpath with a test — it's the only new behavior a regression could silently undo (card-endpoints-test thread). - Delete the now-dead
original.metafallbacks inpatchCardInstance(nit; realm.ts thread).
Adjacent, out of scope: containsMany per-item polymorphic overrides cannot influence relationship-path resolution — parseRelationshipKey strips the index (its own comment calls the transform lossy) and isMeta rejects the array-shaped meta.fields entry — so a relationship declared only on an item's polymorphic subtype under a resolvable declared type still drops silently. This PR narrows that family of loss (unexported declared types now preserve), but the lossy key-cleaning root remains; noted for whoever picks up the schema-driven key parsing the existing comment already asks for.
Preview deploymentsHost Test Results 1 files ±0 1 suites ±0 3h 10m 36s ⏱️ -45s Results for commit dce7f34. ± Comparison against earlier commit 78f8043. Realm Server Test Results 1 files ±0 1 suites ±0 17m 33s ⏱️ -20s Results for commit dce7f34. ± Comparison against earlier commit 78f8043. |
…tests The declared-linksToMany fan-out had the same flattening the unresolvable branch fixed: normalizeRelationship deletes array-valued data without deriving links, so entries with no relationship-level links lost their targets. Derive each indexed entry from its own resource identifier instead. The merge-base fallbacks for a missing meta/adoptsFrom are dead code behind the isCardResource guard, which requires both. A PATCH against a corrupt stored file now has a test pinning the fail-without- overwrite behavior. Host test expectations updated for the stored-file merge base: files keep their author's key order (the index's jsonb round-trip alphabetized them) and stay sparse. The linksToMany-removal test previously patched the same single pet its fixture started with — a semantic no-op that only wrote because the index form differed cosmetically; it now starts with two pets and removes one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Server-side instance writes (POST, PATCH,
/_atomic) re-serialize the card document through the definition-driven file serializer before it lands on disk. The serializer walks the card's storedDefinitionto normalize each field, and for compound (FieldDef-backed) fields it descends into a child definition looked up from the field'sfieldOrCardcode ref.The definition store only holds entries for exported classes. A compound field whose declared type is an unexported FieldDef carries a
fieldOf/ancestorOfcode ref, which can't be resolved to a definition. The serializer treated that unresolvable lookup as "this field has no fields": everycontainsManyentry serialized to{}(array length preserved, attribute data gone), acontainsvalue was dropped outright, and any relationship whose dotted path crosses such a field was dropped. Editing such a card in the UI or pushing it through/_atomicsilently wiped its compound field data on disk.The serializer now passes values through verbatim whenever a compound field's child definition can't be resolved, and preserves relationships whose dotted path crosses such a field. Schema-aware normalization (relativizing inner code refs, stripping computed fields) is skipped for exactly that subtree — an unnormalized code ref is recoverable, dropped field data is not. Fields the definition doesn't declare at all are still dropped, as before.
The PATCH handler had a second, independent path to the same symptom: it built its merge base from the index's
pristine_docrather than the stored file. The index is downstream of the file and can lag it — an in-flight or backlogged index job hands back stale state, and merging a patch over a stale base silently reverts every field the index hasn't caught up on. The merge base is now the stored source file, read inside the same per-realm write lock as the write itself.Behavioral consequences of the merge-base change: a PATCH against an instance whose file exists but isn't indexed yet now succeeds instead of returning 404; a PATCH against a file that isn't parseable as a card document fails with a clear error instead of overwriting the file with just the patch; and the stored file keeps only the fields the file and the patch actually specify — PATCH no longer materializes the index's schema-defaulted nulls (e.g. an untouched
cardInfo) into the file, matching how POST and/_atomicwrites already store documents. GET responses are unaffected since they serialize from the index, which materializes the full field shape.New realm-server tests cover the three surfaces: PATCH preservation of
containsMany/containsvalues and nested relationships backed by an unexported FieldDef, PATCH merging against a file the index hasn't caught up with, and/_atomic?waitForIndex=truepreserving compound entries pushed as raw source.🤖 Generated with Claude Code
https://claude.ai/code/session_01BzRJdC6yxAU8oXY42qn4pW